Skip to content

Adjust to new JSON ABI format #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Adjust to new JSON ABI format #2

wants to merge 7 commits into from

Conversation

topolarity
Copy link
Collaborator

Expect some more touch-ups on Monday, but this should be the core pieces.

There are some significant changes to how mangling works in here, but hopefully this feels mostly in-spirit with the existing behavior. As a bonus, we now support recursive types! (which require forward declarations / pointer-indirection in C, of course)

@timholy
Copy link
Contributor

timholy commented Aug 1, 2025

Thanks! I'll review shortly. As you probably know, there is no "user base" for this package yet, so feel free to "break" anything you think should be improved.

Also add a struct definition to be able to conveniently document the
various bits of output data.
This should remove some gnarly-looking underscores.
@topolarity topolarity marked this pull request as ready for review August 4, 2025 17:18
Copy link
Contributor

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. A few things to think about, and then merge when you feel ready.

@@ -5,10 +5,12 @@ authors = ["Tim Holy <[email protected]> and contributors"]

[deps]
Graphs = "86223c79-3864-5bf0-83f7-82e725a168b6"
JSON = "682c06a0-de6a-54ab-a142-c8b1cf79cde6"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON3/StructTypes might automate some of the json->julia parsing, but I am not sure there would be any real gain and this seems fine to me.

@@ -3,10 +3,10 @@ module JuliaLibWrapping
using OrderedCollections
using Graphs

export parselog, wrapper
export CProject
export import_abi_info, wrapper
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an improvement in naming, thanks


struct PrimitiveTypeDesc
# TODO: support custom primitive types?
name::String
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to include bits?

end

function build_type_graph(typedescs::OrderedDict{Int, TypeDesc};
pointer_filter::Function)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to make pointer_filter the first positional argument so that one could use a do block? https://docs.julialang.org/en/v1/manual/style-guide/#Write-functions-with-argument-ordering-similar-to-Julia-Base

# therefore have to use forward-declarations instead of just sorting declarations.
recursive_types = BitSet()

full_type_graph = build_type_graph(typedescs; pointer_filter = (id)->true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
full_type_graph = build_type_graph(typedescs; pointer_filter = (id)->true)
full_type_graph = build_type_graph(typedescs; pointer_filter = Returns(true))

end

"""
entrypoints, typedescs, forward_declarations = parselog(filename::String)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
entrypoints, typedescs, forward_declarations = parselog(filename::String)
abi_info = import_abi_info(filename::String)

forward_declared::BitSet
# A vector of exposed functions from the imported ABI
entrypoints::Vector{MethodDesc}
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this benefit from having a Base.show method? And should the comments be moved into a docstring?

@test collect(keys(sorted)) == Int[5,2,1,4,3]
else
@test false # unexpected forward declarations
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants